Skip to content

fix(pi): notify review URL in RPC sessions - #1407

Open
Shujakuinkuraudo wants to merge 1 commit into
backnotprop:mainfrom
Shujakuinkuraudo:fix/pi-rpc-url-notice
Open

fix(pi): notify review URL in RPC sessions#1407
Shujakuinkuraudo wants to merge 1 commit into
backnotprop:mainfrom
Shujakuinkuraudo:fix/pi-rpc-url-notice

Conversation

@Shujakuinkuraudo

Copy link
Copy Markdown

Summary

Pi Web runs extensions with ExtensionContext.mode === "rpc". In this mode,
Plannotator's isRemoteSession() can remain false even though the user cannot
directly access the browser launched by the Pi Web host process.

openBrowserForServer() currently notifies the URL only for remote sessions or
when browser launch reports failure. A successful browser spawn in the Pi Web
host can therefore hide the URL from the RPC session.

Treat RPC extension contexts like remote sessions for the purpose of URL
notification:

- if (isRemoteSession()) {
+ if (ctx.mode === "rpc" || isRemoteSession()) {
    ctx.ui.notify(`[Plannotator] ${serverUrl}`, "info");
  }

Behavior

  • Local interactive sessions keep their existing behavior.
  • Existing SSH and PLANNOTATOR_REMOTE sessions keep their existing behavior.
  • Pi Web RPC sessions receive the review URL through ctx.ui.notify() even
    when the browser launcher reports success.
  • Server binding, hostname selection, ports, authentication, browser launch,
    and port forwarding are unchanged.
  • This does not make the random review port directly reachable through Pi Web;
    it only exposes the URL to the user so an explicitly configured access path
    can be used.

Test plan

  • Added a regression test for an RPC context with PLANNOTATOR_REMOTE=0.
  • The test points BROWSER at the current Node executable, so no real browser
    is opened while the launcher still reports success.
  • The test verifies that the RPC context receives the [Plannotator] <url>
    notification.
  • git diff --check passes.
  • The full Bun test suite could not be run locally because Bun is not installed
    in the development environment; GitHub Actions should run the repository's
    standard checks.

Closes #1406

@backnotprop

Copy link
Copy Markdown
Owner

Thanks for the careful writeup on both the issue and the PR. The diagnosis is right: ctx.mode is the documented discriminator (ExtensionMode = "tui" | "rpc" | "json" | "print", required, and hasUI is true in both TUI and RPC so it cannot stand in for it). The one-line change typechecks and does fix the submit_plan and archive paths.

Three things before this can land.

  1. It probably does not fix the three commands Pi Web RPC sessions do not show the Plannotator review URL #1406 names. apps/pi-extension/index.ts:182-194 has a second, independent gate:

    function sessionOpenedMessage(label: string, url: string): string {
    if (!isRemoteSession()) return ${label}. You can keep chatting while it runs.;

called at index.ts:659, :958 and :1048 for /plannotator-review, /plannotator-annotate and /plannotator-last. Its own comment says the after-turn notify inside openBrowserForServer "fires too late to render". So under RPC those three still show a URL-less message. Could you check in a real Pi Web session whether the notify renders there, and extend sessionOpenedMessage if it does not?

  1. New stale-ctx exposure. ctx.mode is a guarded getter that throws after session replacement, which this repo documents at apps/pi-extension/current-pi-session.ts:40-66 and wraps in isCtxAlive(). Today the local browser-opened path reads no ctx property at all; after this change ctx.mode is read on every call. Via startBrowserDecisionSession that turns into misleading console.error noise (plannotator-browser.ts:266-272), and via openBrowserAndWait there is no catch, so an archive open that used to succeed now answers request.respond({ status: "error" }) at plannotator-events.ts:429. Please guard the read.

  2. The test spawns a real process. process.execPath is not in NOOP_BROWSER_VALUES (server/network.ts:17), so network.ts:341-351 runs spawn(, ["http://localhost:4321"], { detached: true }) on macOS and Linux CI. The file header promises these tests never spawn a browser. I know the obvious alternative is worse (a sentinel BROWSER with PLANNOTATOR_REMOTE=0 falls through to resolveUseGlimpse, which reads the real ~/.plannotator/config.json), so the cleaner fix is a seam: extract a pure predicate like shouldAnnounceUrl(mode, isRemote, opened) and test that with no env mutation and no spawn.

Heads up that #1184 (open) rewrites this exact hunk to notify unconditionally, which covers RPC, Codex desktop and any future host without reading ctx.mode at all. That is the same argument your issue makes one host later, and it is probably where this should converge. Worth syncing with that PR rather than both of you landing on the same four lines.

AI-assisted (Claude) under maintainer direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pi Web RPC sessions do not show the Plannotator review URL

2 participants